Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate types for UserApplicationId and ApplicationId. #3582

Closed
wants to merge 1 commit into from

Conversation

deuszx
Copy link
Contributor

@deuszx deuszx commented Mar 17, 2025

Motivation

We want to fix and expand our notion of Address (currently Owner, GenericApplicationId, etc.) to different type of addresses (32-byte Linera/Solana, 20-byte EVM). In order to do that we need to prepare the code for the introduction of new variants.

Proposal

Add a separate type for UserApplicationId(hash) vs ApplicationId (a hash + type-safe ABI). Previously the presence of A type parameter in "address" enum made it more difficult to refactor. After this PR we have separate types for application's address and application with ABI (used mostly in tests and CLI now).

Note: This is an alternative to #3570 . Where I kept the UserApplicationId name but promoted it to a newtype.

Test Plan

CI should catch regressions.

Release Plan

  • Nothing to do / These changes follow the usual release cycle.

Links

@deuszx deuszx marked this pull request as ready for review March 17, 2025 11:09
@deuszx deuszx requested review from afck and ma2bd March 17, 2025 11:13
Copy link
Contributor

@afck afck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will we need both types in the long run?

@deuszx
Copy link
Contributor Author

deuszx commented Mar 17, 2025

Will we need both types in the long run?

They are used in different contexts:

  • UserApplicationId is just a wrapper around the address
  • Application carries the type-safe ABI that's useful in calls and queries

Maybe we will be able to get rid of UserApplicationId at some point and replace it with generic Address type (see #3576 for work in that direction).

Copy link
Contributor

@ma2bd ma2bd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like ApplicationId<()> is used both internally and in the SDK, and that we're not really going to replace these occurrences by Address. (EDIT: actually in the works here #3592)

Ofc we could revisit ApplicationId<A=()>. IMHO the most rigorous choice would be ApplicationId and TypedApplicationId<A> (akin to your previous PR except a different name for the type variant). However, I don't see the issue with ApplicationId<A=()> objectively speaking, and why users would be burdened with a longer name in this case.

@ma2bd
Copy link
Contributor

ma2bd commented Mar 18, 2025

Also this doesn't look to be a requirement for #3576, is it?

@deuszx deuszx force-pushed the alt-separate-application-id branch 3 times, most recently from 0085897 to 2a5eec7 Compare March 20, 2025 20:31
@deuszx deuszx force-pushed the alt-separate-application-id branch from 2a5eec7 to c81d3eb Compare March 20, 2025 20:58
Copy link
Contributor

@ma2bd ma2bd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ended up complicating the refactoring of AccountOwner so I propose to go in the other direction for now (#3626) and reevalutate later

@deuszx
Copy link
Contributor Author

deuszx commented Mar 24, 2025

Postponed for the time being and addressed (partially) in #3626

@deuszx deuszx closed this Mar 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants